Skip to content

switch to ruff #5218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

switch to ruff #5218

wants to merge 5 commits into from

Conversation

gvwilson
Copy link
Contributor

@gvwilson gvwilson commented Jun 5, 2025

  • Remove requires-optional.txt and test_requirements/* (no longer used).
  • Convert commands.py to use argparse instead of hand-rolled.
  • Rationalize imports in codegen/__init__.py (all at top level).
  • Update "dev" section of pyproject.toml to use ruff instead of black.
  • Update pyproject.toml to install inflect and requests.
  • Modify uv.lock file.
  • Modify code reformatting in codegen/__init__.py to use ruff instead of black.
  • Add new function to use ruff to reformat existing code (run python commands.py format).
  • Add new function to use ruff to check code (run python commands.py lint).
  • Regenerate and reformat code.

This PR has been rebuilt on top of #5214 to include recent changes to validator generation.

This PR currently fails the build because Circle CI is expecting requires-optional.txt, which this PR removes.
We need to decide if we're leaving build on Circle CI or moving it to GitHub.

@gvwilson gvwilson requested a review from emilykl June 5, 2025 13:48
@emilykl
Copy link
Contributor

emilykl commented Jun 5, 2025

@gvwilson big picture comments --

  • I have a STRONG preference for also committing the ruff-formatted files as part of this PR. (To avoid cluttering up this PR, the formatting changes themselves could be done on a separate branch which branches off of this one.) But if we don't do the actual formatting step as part of this PR, we'll miss issues and then have to fix them later once this is on main.

  • The CONTRIBUTING.md needs to be updated to remove references to requires-optional.txt, and it needs some general updates as well. I'd be happy to work on that.

  • Could we add the uv.lock in a separate PR? I'm not opposed, but CONTRIBUTING.md needs to be updated to explain how to use the lockfile in development, otherwise things will get messy.

@gvwilson gvwilson force-pushed the switch-to-ruff branch 4 times, most recently from af41d30 to 9c60129 Compare June 6, 2025 16:23
CONTRIBUTING.md Outdated
## Have Questions about Plotly?
We use Git and GitHub to manage our project;
if you are not familiar with them,
there are great resources like <http://try.github.io/> to get your started.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
there are great resources like <http://try.github.io/> to get your started.
there are great resources like <http://try.github.io/> to get you started.

CONTRIBUTING.md Outdated

```bash
$ python commands.py updateplotlyjs
pytest tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pytest tests
python -m pytest tests

CONTRIBUTING.md Outdated
Comment on lines 261 to 184
$ npm pack
$ mv plotly.js-*.tgz plotly.js.tgz
To test `go.FigureWidget` locally, you'll need to generate the JavaScript bundle as follows:

# In your plotly.py/ directory:
$ python commands.py updateplotlyjsdev --local /path/to/your/plotly.js/
```
cd js
npm install && npm run build
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what I should be deleting here - can you please re-check after I push an update and let me know?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry that was ambiguous. I'm suggesting to delete lines 179-184 on your branch in CONTRIBUTING.md. The following lines convey the same information and are more clear, I think.

@rl-utility-man
Copy link
Contributor

rl-utility-man commented Jun 10, 2025

What would you think of adding an item to the pull request template documentations section that recommends:

[ ] start Python code blocks with ```python?

It's something that is not obvious in the checklist and that I've forgotten to do a couple times. Forgetting to do so can prevent the example from displaying.

gvwilson added 2 commits June 10, 2025 13:33
-   Remove `requires-optional.txt` and `test_requirements/*` (no longer used).
-   Convert `commands.py` to use `argparse` instead of hand-rolled.
-   Rationalize imports in `codegen/__init__.py` (all at top level).
-   Update `"dev"` section of `pyproject.toml` to use `ruff` instead of `black`.
-   Update `pyproject.toml` to install `inflect` and `requests`.
-   Modify `uv.lock` file.
-   Modify code reformatting in `codegen/__init__.py` to use `ruff` instead of `black`.
-   Add new function to use `ruff` to reformat existing code (run `python commands.py format`).
-   Add new function to use `ruff` to check code (run `python commands.py lint`).

This PR currently fails the build because Circle CI is expecting `requires-optional.txt`, which this PR removes.
We need to decide if we're leaving build on Circle CI or moving it to GitHub.
@gvwilson gvwilson self-assigned this Jun 10, 2025
@gvwilson gvwilson added feature something new P1 needed for current cycle labels Jun 10, 2025
A previous PR in this branch removed `test_requirements/requirements_core.txt`
and `test_requirements/requirements_optional.txt`. @emilylk pointed out that
these files are used in Circle CI to set up a minimal environment for running
`tests/test_core/*` and then a larger environment for running the other tests
(the idea being to prevent extra dependencies from sneaking into plotly.py).
This PR divides dependencies into `dev_core` and `dev_optional` with `dev`
essentially aliasing `dev_optional` so that `./.circleci/config.yml` can be
modified to use these for installation.

Note: `uv.lock` reflects `dev_optional`, i.e., pins *all* dependencies.
@emilykl
Copy link
Contributor

emilykl commented Jun 11, 2025

@gvwilson Add this line to the dev_optional extra in pyproject.toml; we had it in test_requirements/requires_optional.txt and I believe it will fix the Python 3.8 failure in the CI.

"fiona<=1.9.6;python_version<='3.8'"  # fiona>1.9.6 is not compatible with geopandas<1; geopandas>=1 is not compatible with python 3.8

-   Modify `pyproject.toml` to have a separate `dev_build` section
    with packages needed for building the plotly package.
-   Replace uses of `uv pip` with `uv sync` in `.circleci/config.yml`.
@gvwilson gvwilson force-pushed the switch-to-ruff branch 3 times, most recently from e908dad to 6ce94b2 Compare June 11, 2025 20:35
-   Modify `pyproject.toml` to exclude `plotly/graph_objs` from checking for now.
-   Modify `pyproject.toml` to include `anywidget` in core dependencies
    (because `plotly/basewidget.py` needs it).
-   Clean up several hundred complaints from `ruff` about unused variables and imports.
-   Clean up cases in `tests/**/*.py` where the same function name was used for several tests
    (which meant that only the last one defined was actually run).
-   Reformat code after recent changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants